Conversation
This reverts commit a0a2ad9.
📝 WalkthroughWalkthroughReplaces UnityWebRequest with a shared HttpClient for editor networking, makes avatar UI controls visible, adds an Uninstall menu command, introduces YouTube thumbnail/video models and sanitization, adjusts runtime timing/lifecycle and browser prefab selection, trims allowed-member stubs, and removes several debug logs. Changes
Sequence Diagram(s)sequenceDiagram
participant EditorUI as Editor UI / BuilderWindow
participant TokenStore as TokenProvider
participant Http as HttpClient (shared)
participant API as SideQuest API
EditorUI->>TokenStore: request token (if needed)
TokenStore-->>EditorUI: token
EditorUI->>Http: construct HttpRequestMessage (with token + headers)
Http->>API: send HTTP request
API-->>Http: HTTP response (status + payload)
Http-->>EditorUI: response content / status
EditorUI->>EditorUI: deserialize JSON / LoadImage(byte[]) / invoke callbacks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
Editor/SDKInitialiseOnLoad.cs (1)
39-39: Remove the commented-out uninstaller scaffold.The real uninstall flow now lives in
Editor/Resources/Builder/MenuUtils.cs. Keeping a second, fully commented copy here has already started to drift (pacakge.json, different cleanup steps) and makes the initializer much harder to scan.Also applies to: 73-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Editor/SDKInitialiseOnLoad.cs` at line 39, Remove the commented-out uninstaller scaffold from Editor/SDKInitialiseOnLoad.cs: delete the commented line "// CreateUninstaller();" and the larger commented block spanning the existing uninstaller scaffold (previously lines 73-156) so the initializer no longer contains the outdated, duplicated uninstall logic; rely on the real uninstall flow in Editor/Resources/Builder/MenuUtils.cs and ensure no other commented duplicate functions (e.g., CreateUninstaller) remain in this file.Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs (2)
719-759:HttpRequestMessageandHttpResponseMessagenot disposed.Same resource management concern as other methods. Both objects should be disposed after use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs` around lines 719 - 759, JsonGet currently creates an HttpRequestMessage (request) and obtains an HttpResponseMessage (response) without disposing them; wrap request creation and the code path that uses response in proper disposal (e.g., using blocks or try/finally Dispose calls) so request is disposed after SendAsync and response (and its Content if needed) is disposed after reading the content or on all error/early-return paths; update symbols referenced (JsonGet, request, _httpClient.SendAsync, response, response.Content, readTask) to ensure every exit/exception path disposes those objects.
687-717:HttpResponseMessageandStringContentnot disposed.Both the content and response should be disposed. The
StringContentpassed toPostAsyncand the returnedHttpResponseMessageimplementIDisposable.Suggested pattern
private IEnumerator PostFormEncodedStringNoAuth<T>(string urlPath, string data, Action<T> OnCompleted, Action<Exception> OnError) { - var content = new StringContent(data, Encoding.UTF8, "application/x-www-form-urlencoded"); - var task = _httpClient.PostAsync(new Uri(Config.RootApiUri, urlPath), content); + using var content = new StringContent(data, Encoding.UTF8, "application/x-www-form-urlencoded"); + var task = _httpClient.PostAsync(new Uri(Config.RootApiUri, urlPath), content); while (!task.IsCompleted) yield return null; // ... error handling ... - var response = task.Result; + using var response = task.Result; // ... rest of method🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs` around lines 687 - 717, In PostFormEncodedStringNoAuth, both the created StringContent (content) and the HttpResponseMessage (response from _httpClient.PostAsync) must be disposed; wrap the StringContent creation in a using (or try/finally) and similarly dispose the response after you finish using it (i.e. after awaiting/finishing response.Content.ReadAsStringAsync and storing the result) to avoid leaking resources; ensure you still handle task faults, non-success status codes, and call OnError/OnCompleted as before while disposing in all control paths (use using/try-finally around content and a using/try-finally around response/readTask/result).Editor/Resources/Builder/BuilderWindow.cs (3)
1189-1201:HttpResponseMessagenot disposed.The response from
GetAsyncshould be disposed to release underlying resources (connection back to pool). Without disposal, connections may not be returned to the pool promptly.Suggested fix using try-finally
public IEnumerator Json<T>(string url, Action<T> callback) { var task = _httpClient.GetAsync(url); while (!task.IsCompleted) yield return null; if (task.IsFaulted) throw task.Exception.InnerException ?? task.Exception; var response = task.Result; - if (!response.IsSuccessStatusCode) - throw new System.Exception(response.StatusCode + ": " + response.ReasonPhrase); - var readTask = response.Content.ReadAsStringAsync(); - while (!readTask.IsCompleted) yield return null; - if (readTask.IsFaulted) throw readTask.Exception.InnerException ?? readTask.Exception; - callback(JsonUtility.FromJson<T>(readTask.Result)); + try + { + if (!response.IsSuccessStatusCode) + throw new System.Exception(response.StatusCode + ": " + response.ReasonPhrase); + var readTask = response.Content.ReadAsStringAsync(); + while (!readTask.IsCompleted) yield return null; + if (readTask.IsFaulted) throw readTask.Exception.InnerException ?? readTask.Exception; + callback(JsonUtility.FromJson<T>(readTask.Result)); + } + finally + { + response.Dispose(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Editor/Resources/Builder/BuilderWindow.cs` around lines 1189 - 1201, The HttpResponseMessage returned in Json<T>(string url, Action<T> callback) is never disposed; wrap the code that inspects response and reads its content so the response is always disposed (e.g., assign response = task.Result, start the readTask, await its completion, and then in a try/finally or using-equivalent ensure response.Dispose() in the finally block). Make sure you read response.Content.ReadAsStringAsync() and obtain readTask.Result before disposing the response, and preserve existing error handling (throw on non-success status or task faults).
1203-1231:HttpRequestMessageandHttpResponseMessagenot disposed.Both the request and response objects implement
IDisposableand should be disposed to properly release resources. This is especially important for theStringContentwithin the request.Suggested fix pattern
public IEnumerator Json<T>(string url, T postData, Action<string> callback, Dictionary<string, string> headers = null) { var json = JsonUtility.ToJson(postData); - var request = new HttpRequestMessage(HttpMethod.Post, url) - { - Content = new StringContent(json, Encoding.UTF8, "application/json") - }; + using var request = new HttpRequestMessage(HttpMethod.Post, url) + { + Content = new StringContent(json, Encoding.UTF8, "application/json") + }; if (headers != null) { foreach (var header in headers) { if (!header.Key.Equals("Content-Type", StringComparison.OrdinalIgnoreCase)) request.Headers.TryAddWithoutValidation(header.Key, header.Value); } } var task = _httpClient.SendAsync(request); while (!task.IsCompleted) yield return null; if (task.IsFaulted) throw task.Exception.InnerException ?? task.Exception; - var response = task.Result; + using var response = task.Result; // ... rest of the methodNote:
usingdeclarations require C# 8.0+. If using an older version, wrap in try-finally blocks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Editor/Resources/Builder/BuilderWindow.cs` around lines 1203 - 1231, The Json<T> coroutine creates an HttpRequestMessage (and a StringContent) and uses HttpResponseMessage from _httpClient.SendAsync without disposing them; wrap the HttpRequestMessage (and its Content) in a using (or try/finally Dispose) and also ensure the HttpResponseMessage is disposed after you finish reading its content (read response.Content.ReadAsStringAsync() and await/complete that before disposing); update the code around HttpRequestMessage creation, _httpClient.SendAsync call, and handling of response/response.Content (symbols: Json<T>, HttpRequestMessage, StringContent, _httpClient.SendAsync, HttpResponseMessage, response.Content.ReadAsStringAsync) so all IDisposable objects are properly disposed while preserving the existing error/exception handling and callback behavior.
53-54: Consider configuring timeout for consistency.The
HttpClientinstance here lacks timeout configuration, whileSqEditorAppApi.cssets a 10-minute timeout. For image downloads and API calls, consider adding a similar timeout configuration or using a sharedHttpClientinstance across both files to ensure consistent behavior.Suggested configuration
- private static readonly HttpClient _httpClient = new HttpClient(); + private static readonly HttpClient _httpClient = new HttpClient() + { + Timeout = TimeSpan.FromMinutes(5) + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Editor/Resources/Builder/BuilderWindow.cs` around lines 53 - 54, The static HttpClient _httpClient in BuilderWindow.cs has no timeout set; update it to use the same 10-minute timeout as SqEditorAppApi.cs (or refactor to reuse the shared HttpClient used by SqEditorAppApi) by configuring the Timeout property on the shared/static HttpClient instance (refer to the _httpClient field in BuilderWindow and the HttpClient usage in SqEditorAppApi.cs) so both image downloads and API calls have consistent timeout behavior.Editor/Scripts/VisualScripting/VsStubsAllowed.cs (1)
12635-12798: Consider centralizing the 29 inherited Unity members shared across these four NavMesh types.This block duplicates the same inherited Object/Component surface (29 members) across NavMeshLink, NavMeshModifier, NavMeshModifierVolume, and NavMeshSurface. If the allowlist matcher supports it, extracting these common members into a reusable list or type-level pattern would reduce the maintenance burden if Unity adds or renames inherited members in future releases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Editor/Scripts/VisualScripting/VsStubsAllowed.cs` around lines 12635 - 12798, The four types Unity.AI.Navigation.NavMeshLink, NavMeshModifier, NavMeshModifierVolume, and NavMeshSurface repeat the same 29 inherited Object/Component members; extract those shared strings into a single reusable list (e.g., commonUnityComponentMembers) and reuse/concatenate it when building the allowlist entries for each of the four types (replace the duplicated block in the NavMeshLink, NavMeshModifier, NavMeshModifierVolume, and NavMeshSurface entries with a reference to that shared list or a type-level pattern), so future changes to inherited Unity members only need to be made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Editor/Resources/Builder/BuilderWindow.cs`:
- Around line 376-384: In the Texture coroutine, check the boolean return value
of Texture2D.LoadImage after creating tex and loading task.Result; if LoadImage
returns false, dispose/destroy the invalid Texture2D, log or surface the error,
and invoke the provided callback with null (or throw a descriptive exception)
instead of passing an invalid texture; update the Texture method (the coroutine
named Texture and its callback usage) to handle the failed load path cleanly so
callers can detect and handle image decoding failures.
In `@Editor/Resources/Builder/MenuUtils.cs`:
- Around line 50-68: UninstallBanter() currently deletes the local package
folders but leaves file: entries in Packages/manifest.json, causing broken
references; update UninstallBanter() to load Packages/manifest.json, remove the
dependencies for "com.sidequest.ora" and the "com.basis.bundlemanagement",
"com.basis.sdk", "com.basis.odinserializer" keys (the same packages added by
InitialiseOnLoad.ImportOraPackage() and ImportBasisPackages()), write the
manifest back (preserving JSON formatting), then remove the directories and keep
the existing EditUtils.RemoveCompileDefine calls and
EditorApplication.OpenProject(Directory.GetCurrentDirectory()) so the project is
reopened with a cleaned manifest.
- Around line 45-49: The busy-wait loop that polls RemoveRequest.IsCompleted
after calling Client.Remove("com.sidequest.banter") blocks the editor update
pump; instead register a callback on EditorApplication.update to poll the
RemoveRequest until IsCompleted, then unregister the callback and continue
processing. Locate the call site using Client.Remove and the RemoveRequest
variable name, replace the while(!request.IsCompleted) { } spin with an
EditorApplication.update delegate that checks request.IsCompleted each tick,
calls whatever follow-up logic is needed when completed, and removes itself from
EditorApplication.update to avoid leaking the callback.
In `@Runtime/Scripts/BanterLink/BanterLink.cs`:
- Around line 147-153: Remove the hard-coded await new WaitForSeconds(2) and
instead rely on the concrete readiness check (do not introduce arbitrary
sleeps); use the existing WaitUntil that checks scene.loaded (and call
scene.SetLoaded() at the appropriate time before/inside that check) so emission
of UNITY_LOADED happens immediately when scene.loaded becomes true—modify the
block around WaitForSeconds/WaitUntil (references: WaitForSeconds, WaitUntil,
scene.SetLoaded(), scene.loaded, LogLine.Do) to await the readiness condition
only and remove the two-second delay.
- Around line 136-141: Restore handling for APICommands.DOM_READY by re-enabling
the branch that sets scene.state = SceneState.DOM_READY, invokes
scene.events.OnDomReady, and calls scene.SetLoaded() so DOM readiness
contributes to scene.isReady; locate the commented block around
APICommands.DOM_READY in BanterLink.cs and uncomment or reimplement that exact
behavior. If instead DOM_READY is intended to be retired, update the readiness
contract in the same change by modifying BanterScene.isReady and any listeners
that depend on OnDomReady to use the new readiness signal (ensure all references
to SceneState.DOM_READY, scene.events.OnDomReady.Invoke(), and scene.SetLoaded()
are removed or replaced consistently).
---
Nitpick comments:
In `@Editor/Resources/Builder/BuilderWindow.cs`:
- Around line 1189-1201: The HttpResponseMessage returned in Json<T>(string url,
Action<T> callback) is never disposed; wrap the code that inspects response and
reads its content so the response is always disposed (e.g., assign response =
task.Result, start the readTask, await its completion, and then in a try/finally
or using-equivalent ensure response.Dispose() in the finally block). Make sure
you read response.Content.ReadAsStringAsync() and obtain readTask.Result before
disposing the response, and preserve existing error handling (throw on
non-success status or task faults).
- Around line 1203-1231: The Json<T> coroutine creates an HttpRequestMessage
(and a StringContent) and uses HttpResponseMessage from _httpClient.SendAsync
without disposing them; wrap the HttpRequestMessage (and its Content) in a using
(or try/finally Dispose) and also ensure the HttpResponseMessage is disposed
after you finish reading its content (read response.Content.ReadAsStringAsync()
and await/complete that before disposing); update the code around
HttpRequestMessage creation, _httpClient.SendAsync call, and handling of
response/response.Content (symbols: Json<T>, HttpRequestMessage, StringContent,
_httpClient.SendAsync, HttpResponseMessage, response.Content.ReadAsStringAsync)
so all IDisposable objects are properly disposed while preserving the existing
error/exception handling and callback behavior.
- Around line 53-54: The static HttpClient _httpClient in BuilderWindow.cs has
no timeout set; update it to use the same 10-minute timeout as SqEditorAppApi.cs
(or refactor to reuse the shared HttpClient used by SqEditorAppApi) by
configuring the Timeout property on the shared/static HttpClient instance (refer
to the _httpClient field in BuilderWindow and the HttpClient usage in
SqEditorAppApi.cs) so both image downloads and API calls have consistent timeout
behavior.
In `@Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs`:
- Around line 719-759: JsonGet currently creates an HttpRequestMessage (request)
and obtains an HttpResponseMessage (response) without disposing them; wrap
request creation and the code path that uses response in proper disposal (e.g.,
using blocks or try/finally Dispose calls) so request is disposed after
SendAsync and response (and its Content if needed) is disposed after reading the
content or on all error/early-return paths; update symbols referenced (JsonGet,
request, _httpClient.SendAsync, response, response.Content, readTask) to ensure
every exit/exception path disposes those objects.
- Around line 687-717: In PostFormEncodedStringNoAuth, both the created
StringContent (content) and the HttpResponseMessage (response from
_httpClient.PostAsync) must be disposed; wrap the StringContent creation in a
using (or try/finally) and similarly dispose the response after you finish using
it (i.e. after awaiting/finishing response.Content.ReadAsStringAsync and storing
the result) to avoid leaking resources; ensure you still handle task faults,
non-success status codes, and call OnError/OnCompleted as before while disposing
in all control paths (use using/try-finally around content and a
using/try-finally around response/readTask/result).
In `@Editor/Scripts/VisualScripting/VsStubsAllowed.cs`:
- Around line 12635-12798: The four types Unity.AI.Navigation.NavMeshLink,
NavMeshModifier, NavMeshModifierVolume, and NavMeshSurface repeat the same 29
inherited Object/Component members; extract those shared strings into a single
reusable list (e.g., commonUnityComponentMembers) and reuse/concatenate it when
building the allowlist entries for each of the four types (replace the
duplicated block in the NavMeshLink, NavMeshModifier, NavMeshModifierVolume, and
NavMeshSurface entries with a reference to that shared list or a type-level
pattern), so future changes to inherited Unity members only need to be made in
one place.
In `@Editor/SDKInitialiseOnLoad.cs`:
- Line 39: Remove the commented-out uninstaller scaffold from
Editor/SDKInitialiseOnLoad.cs: delete the commented line "//
CreateUninstaller();" and the larger commented block spanning the existing
uninstaller scaffold (previously lines 73-156) so the initializer no longer
contains the outdated, duplicated uninstall logic; rely on the real uninstall
flow in Editor/Resources/Builder/MenuUtils.cs and ensure no other commented
duplicate functions (e.g., CreateUninstaller) remain in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 884f27af-e9b8-4a0d-8331-e091c6515ac4
⛔ Files ignored due to path filters (1)
Runtime/Objects/Loading/LoadingCage.matis excluded by!**/*.mat
📒 Files selected for processing (18)
Editor/Resources/Builder/BuilderWindow.csEditor/Resources/Builder/BuilderWindow.uxmlEditor/Resources/Builder/MenuUtils.csEditor/Resources/Builder/SideQuest/SqEditorAppApi.csEditor/SDKInitialiseOnLoad.csEditor/Scripts/VisualScripting/VsStubsAllowed.csRuntime/Scripts/BanterLink/BanterLink.csRuntime/Scripts/BanterLink/BanterPipe.csRuntime/Scripts/BanterStarterUpper.csRuntime/Scripts/Models/YtInfo/YtResponseContext.csRuntime/Scripts/Models/YtInfo/YtThumbnail.csRuntime/Scripts/Models/YtInfo/YtThumbnail.cs.metaRuntime/Scripts/Models/YtInfo/YtVideoDetails.csRuntime/Scripts/Models/YtInfo/YtVideoDetails.cs.metaRuntime/Scripts/Scene/BanterScene.csRuntime/Scripts/Scene/Components/BanterBrowser.csRuntime/Scripts/Utils/AddPanelStuff.csRuntime/Scripts/Utils/DontDestroyOnLoad.cs
💤 Files with no reviewable changes (3)
- Runtime/Scripts/Utils/AddPanelStuff.cs
- Runtime/Scripts/BanterStarterUpper.cs
- Runtime/Scripts/BanterLink/BanterPipe.cs
| public IEnumerator Texture(string url, Action<Texture2D> callback) | ||
| { | ||
| using (UnityWebRequest uwr = UnityWebRequestTexture.GetTexture(url)) | ||
| { | ||
| yield return uwr.SendWebRequest(); | ||
| if (uwr.result != UnityWebRequest.Result.Success) | ||
| { | ||
| throw new System.Exception(uwr.error); | ||
| } | ||
| else | ||
| { | ||
| callback(DownloadHandlerTexture.GetContent(uwr)); | ||
| } | ||
| } | ||
| var task = _httpClient.GetByteArrayAsync(url); | ||
| while (!task.IsCompleted) yield return null; | ||
| if (task.IsFaulted) throw task.Exception.InnerException ?? task.Exception; | ||
| var tex = new Texture2D(1, 1); | ||
| tex.LoadImage(task.Result); | ||
| callback(tex); | ||
| } |
There was a problem hiding this comment.
Unchecked LoadImage return value.
Texture2D.LoadImage() returns a boolean indicating success. If the downloaded bytes are not a valid image format, this will fail silently and the callback will receive an invalid texture.
Suggested fix
var tex = new Texture2D(1, 1);
- tex.LoadImage(task.Result);
- callback(tex);
+ if (!tex.LoadImage(task.Result))
+ {
+ UnityEngine.Object.DestroyImmediate(tex);
+ throw new Exception($"Failed to load image from URL: {url}");
+ }
+ callback(tex);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public IEnumerator Texture(string url, Action<Texture2D> callback) | |
| { | |
| using (UnityWebRequest uwr = UnityWebRequestTexture.GetTexture(url)) | |
| { | |
| yield return uwr.SendWebRequest(); | |
| if (uwr.result != UnityWebRequest.Result.Success) | |
| { | |
| throw new System.Exception(uwr.error); | |
| } | |
| else | |
| { | |
| callback(DownloadHandlerTexture.GetContent(uwr)); | |
| } | |
| } | |
| var task = _httpClient.GetByteArrayAsync(url); | |
| while (!task.IsCompleted) yield return null; | |
| if (task.IsFaulted) throw task.Exception.InnerException ?? task.Exception; | |
| var tex = new Texture2D(1, 1); | |
| tex.LoadImage(task.Result); | |
| callback(tex); | |
| } | |
| public IEnumerator Texture(string url, Action<Texture2D> callback) | |
| { | |
| var task = _httpClient.GetByteArrayAsync(url); | |
| while (!task.IsCompleted) yield return null; | |
| if (task.IsFaulted) throw task.Exception.InnerException ?? task.Exception; | |
| var tex = new Texture2D(1, 1); | |
| if (!tex.LoadImage(task.Result)) | |
| { | |
| UnityEngine.Object.DestroyImmediate(tex); | |
| throw new Exception($"Failed to load image from URL: {url}"); | |
| } | |
| callback(tex); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Editor/Resources/Builder/BuilderWindow.cs` around lines 376 - 384, In the
Texture coroutine, check the boolean return value of Texture2D.LoadImage after
creating tex and loading task.Result; if LoadImage returns false,
dispose/destroy the invalid Texture2D, log or surface the error, and invoke the
provided callback with null (or throw a descriptive exception) instead of
passing an invalid texture; update the Texture method (the coroutine named
Texture and its callback usage) to handle the failed load path cleanly so
callers can detect and handle image decoding failures.
| RemoveRequest request = Client.Remove("com.sidequest.banter"); | ||
| while(!request.IsCompleted) | ||
| { | ||
|
|
||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n Editor/Resources/Builder/MenuUtils.cs | sed -n '40,60p'Repository: SideQuestVR/BanterSDK
Length of output: 1033
🏁 Script executed:
# Search for RemoveRequest usage in the codebase to understand the pattern
rg "RemoveRequest" -A 5 -B 2Repository: SideQuestVR/BanterSDK
Length of output: 1260
🌐 Web query:
UnityEditor.PackageManager.Client.Remove RemoveRequest async completion IsCompleted editor update
💡 Result:
Client.Remove(...) returns a RemoveRequest and runs asynchronously—there’s no callback/event on the request itself, so you detect completion by polling request.IsCompleted and then checking request.Status / request.Error. IsCompleted becomes true only when the UPM operation finishes. [1][2]
Typical Editor pattern (poll each editor tick): [3]
using UnityEditor;
using UnityEditor.PackageManager;
using UnityEditor.PackageManager.Requests;
static RemoveRequest _req;
static void RemovePkg(string name)
{
_req = Client.Remove(name);
EditorApplication.update += Tick;
}
static void Tick()
{
if (!_req.IsCompleted) return;
EditorApplication.update -= Tick;
if (_req.Status == StatusCode.Success) UnityEngine.Debug.Log("Removed");
else UnityEngine.Debug.LogError(_req.Error.message);
}If you also need to know when the Editor has applied/compiled/imported the resulting package set (not just when the request completed), subscribe to UnityEditor.PackageManager.Events.registeredPackages (raised after the Editor applies the changed list). [4] Unity notes cases where the change is only effective after a “full editor tick”, so chaining package operations is often done sequentially via EditorApplication.update (or by waiting for registeredPackages). [5]
Sources: [1] Client.Remove doc, [2] Request.IsCompleted/Status/Error, [3] Unity manual example showing the EditorApplication.update + IsCompleted polling pattern, [4] Package Manager Events, [5] Unity Issue Tracker note about changes becoming effective after a full editor tick.
Replace the busy-wait loop with EditorApplication.update callback.
Client.Remove() completes asynchronously during editor updates. The tight loop on lines 46–49 blocks that update pump, freezing the editor and preventing the uninstall from ever completing. Use EditorApplication.update to poll IsCompleted instead.
Safer pattern
RemoveRequest request = Client.Remove("com.sidequest.banter");
- while(!request.IsCompleted)
- {
-
- }
+ EditorApplication.update += WaitForRemove;
+ void WaitForRemove()
+ {
+ if (!request.IsCompleted) return;
+ EditorApplication.update -= WaitForRemove;
+ if (request.Status == StatusCode.Success)
+ {
+ // continue cleanup here
+ }
+ else
+ {
+ Debug.LogError($"Remove failed: {request.Error.message}");
+ }
+ }
+ return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RemoveRequest request = Client.Remove("com.sidequest.banter"); | |
| while(!request.IsCompleted) | |
| { | |
| } | |
| RemoveRequest request = Client.Remove("com.sidequest.banter"); | |
| EditorApplication.update += WaitForRemove; | |
| void WaitForRemove() | |
| { | |
| if (!request.IsCompleted) return; | |
| EditorApplication.update -= WaitForRemove; | |
| if (request.Status == StatusCode.Success) | |
| { | |
| // continue cleanup here | |
| } | |
| else | |
| { | |
| Debug.LogError($"Remove failed: {request.Error.message}"); | |
| } | |
| } | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Editor/Resources/Builder/MenuUtils.cs` around lines 45 - 49, The busy-wait
loop that polls RemoveRequest.IsCompleted after calling
Client.Remove("com.sidequest.banter") blocks the editor update pump; instead
register a callback on EditorApplication.update to poll the RemoveRequest until
IsCompleted, then unregister the callback and continue processing. Locate the
call site using Client.Remove and the RemoveRequest variable name, replace the
while(!request.IsCompleted) { } spin with an EditorApplication.update delegate
that checks request.IsCompleted each tick, calls whatever follow-up logic is
needed when completed, and removes itself from EditorApplication.update to avoid
leaking the callback.
| if(Directory.Exists("Packages/com.basis.bundlemanagement")) | ||
| { | ||
| Directory.Delete("Packages/com.basis.bundlemanagement", true); | ||
| } | ||
| if(Directory.Exists("Packages/com.basis.sdk")) | ||
| { | ||
| Directory.Delete("Packages/com.basis.sdk", true); | ||
| } | ||
| if(Directory.Exists("Packages/com.basis.odinserializer")) | ||
| { | ||
| Directory.Delete("Packages/com.basis.odinserializer", true); | ||
| } | ||
| if (Directory.Exists("Packages/com.sidequest.ora")) | ||
| { | ||
| Directory.Delete("Packages/com.sidequest.ora", true); | ||
| } | ||
| EditUtils.RemoveCompileDefine("BANTER_ORA", new BuildTargetGroup[] { BuildTargetGroup.Android, BuildTargetGroup.Standalone }); | ||
| EditUtils.RemoveCompileDefine("BASIS_BUNDLE_MANAGEMENT", new BuildTargetGroup[] { BuildTargetGroup.Android, BuildTargetGroup.Standalone }); | ||
| EditorApplication.OpenProject(Directory.GetCurrentDirectory()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "MenuUtils.cs" | head -20Repository: SideQuestVR/BanterSDK
Length of output: 104
🏁 Script executed:
find . -type f -name "manifest.json" | head -10Repository: SideQuestVR/BanterSDK
Length of output: 47
🏁 Script executed:
rg -l "ImportOraPackage\|ImportBasisPackages" --type csRepository: SideQuestVR/BanterSDK
Length of output: 47
🏁 Script executed:
cat -n "./Editor/Resources/Builder/MenuUtils.cs" | head -100Repository: SideQuestVR/BanterSDK
Length of output: 3531
🏁 Script executed:
rg "manifest\.json|ImportOraPackage|ImportBasisPackages|InitialiseOnLoad" --type cs -iRepository: SideQuestVR/BanterSDK
Length of output: 928
🏁 Script executed:
git ls-files | grep -i manifestRepository: SideQuestVR/BanterSDK
Length of output: 47
🏁 Script executed:
cat -n "./Editor/SDKInitialiseOnLoad.cs"Repository: SideQuestVR/BanterSDK
Length of output: 19671
Remove the manifest.json entries for the deleted packages.
InitialiseOnLoad.ImportOraPackage() and ImportBasisPackages() add com.sidequest.ora and the com.basis.* packages (com.basis.bundlemanagement, com.basis.sdk, com.basis.odinserializer) as file: dependencies to Packages/manifest.json. Deleting these directories in UninstallBanter() without removing those manifest entries leaves the reopened project with broken package references pointing to missing local packages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Editor/Resources/Builder/MenuUtils.cs` around lines 50 - 68,
UninstallBanter() currently deletes the local package folders but leaves file:
entries in Packages/manifest.json, causing broken references; update
UninstallBanter() to load Packages/manifest.json, remove the dependencies for
"com.sidequest.ora" and the "com.basis.bundlemanagement", "com.basis.sdk",
"com.basis.odinserializer" keys (the same packages added by
InitialiseOnLoad.ImportOraPackage() and ImportBasisPackages()), write the
manifest back (preserving JSON formatting), then remove the directories and keep
the existing EditUtils.RemoveCompileDefine calls and
EditorApplication.OpenProject(Directory.GetCurrentDirectory()) so the project is
reopened with a cleaned manifest.
| // else if (msg.StartsWith(APICommands.DOM_READY)) | ||
| // { | ||
| // scene.state = SceneState.DOM_READY; | ||
| // scene.events.OnDomReady.Invoke(); | ||
| // scene.SetLoaded(); | ||
| // } |
There was a problem hiding this comment.
Restore DOM_READY or update the readiness contract in the same change.
This was the only branch that raised scene.events.OnDomReady and transitioned the scene into SceneState.DOM_READY. With it commented out, that callback never fires, and DOM readiness no longer participates in the scene.isReady progression. If DOM_READY is being retired, BanterScene.isReady and any listeners should be updated in the same PR.
💡 Minimal fix
- // else if (msg.StartsWith(APICommands.DOM_READY))
- // {
- // scene.state = SceneState.DOM_READY;
- // scene.events.OnDomReady.Invoke();
- // scene.SetLoaded();
- // }
+ else if (msg.StartsWith(APICommands.DOM_READY))
+ {
+ scene.state = SceneState.DOM_READY;
+ scene.events.OnDomReady.Invoke();
+ scene.SetLoaded();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // else if (msg.StartsWith(APICommands.DOM_READY)) | |
| // { | |
| // scene.state = SceneState.DOM_READY; | |
| // scene.events.OnDomReady.Invoke(); | |
| // scene.SetLoaded(); | |
| // } | |
| else if (msg.StartsWith(APICommands.DOM_READY)) | |
| { | |
| scene.state = SceneState.DOM_READY; | |
| scene.events.OnDomReady.Invoke(); | |
| scene.SetLoaded(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Runtime/Scripts/BanterLink/BanterLink.cs` around lines 136 - 141, Restore
handling for APICommands.DOM_READY by re-enabling the branch that sets
scene.state = SceneState.DOM_READY, invokes scene.events.OnDomReady, and calls
scene.SetLoaded() so DOM readiness contributes to scene.isReady; locate the
commented block around APICommands.DOM_READY in BanterLink.cs and uncomment or
reimplement that exact behavior. If instead DOM_READY is intended to be retired,
update the readiness contract in the same change by modifying
BanterScene.isReady and any listeners that depend on OnDomReady to use the new
readiness signal (ensure all references to SceneState.DOM_READY,
scene.events.OnDomReady.Invoke(), and scene.SetLoaded() are removed or replaced
consistently).
| await new WaitForSeconds(2); | ||
| await new WaitUntil(() => | ||
| { | ||
| scene.SetLoaded(); | ||
| return scene.loaded; | ||
| }); | ||
| }); | ||
| LogLine.Do(LogLine.banterColor, LogTag.Banter, "After Banter Scene Loaded."); |
There was a problem hiding this comment.
Avoid a hard-coded 2s pause in the load handshake.
Line 147 adds a guaranteed two-second delay before UNITY_LOADED can be emitted, even when the scene is already ready. If this is masking another race, please wait on that concrete condition instead of slowing every load.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Runtime/Scripts/BanterLink/BanterLink.cs` around lines 147 - 153, Remove the
hard-coded await new WaitForSeconds(2) and instead rely on the concrete
readiness check (do not introduce arbitrary sleeps); use the existing WaitUntil
that checks scene.loaded (and call scene.SetLoaded() at the appropriate time
before/inside that check) so emission of UNITY_LOADED happens immediately when
scene.loaded becomes true—modify the block around WaitForSeconds/WaitUntil
(references: WaitForSeconds, WaitUntil, scene.SetLoaded(), scene.loaded,
LogLine.Do) to await the readiness condition only and remove the two-second
delay.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs (1)
532-562:⚠️ Potential issue | 🟠 MajorWrap
HttpResponseMessageandHttpRequestMessageobjects inusingstatements to ensure proper resource disposal.At lines 542, 700, 742, and 788, response objects are left undisposed across all code paths. The first method (
UploadFileWithRetryAsync, lines 542) also leaves the response undisposed before returning or throwing. The other three methods (PostFormEncodedStringNoAuthat 700,JsonGetat 742,JsonPostat 788) additionally leave both request and response objects undisposed across multipleyield breakexits.With a shared static
HttpClient, undisposedHttpResponseMessageobjects don't release their underlying connections back to the connection pool, which can exhaust the pool and cause timeouts on subsequent requests.Also applies to: 692-710, 731-760, 775-814
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs` around lines 532 - 562, Several HTTP request/response objects (HttpRequestMessage and HttpResponseMessage) created in UploadFileWithRetryAsync, PostFormEncodedStringNoAuth, JsonGet and JsonPost are not disposed on all codepaths; update each method to wrap the HttpRequestMessage and the HttpResponseMessage returned by _httpClient.SendAsync(...) in using blocks (or ensure disposal in a try/finally) so responses are always disposed before returning or throwing (including on error/early returns and before any yield break paths); specifically modify the request creation and the var response = await _httpClient.SendAsync(...) sites to ensure both request and response are disposed in every path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs`:
- Around line 475-477: UploadFileToCommunity currently always proceeds to
AttachToCommmunity and invokes OnCompleted even if UploadFileInternal signaled
OnError; change the flow so failures stop the outer coroutine: have
UploadFileInternal communicate success (e.g., change its contract to return a
bool result or set a shared success flag on the _uploadRequest or a local
variable) and in UploadFileToCommunity check that success before yielding
AttachToCommmunity and before invoking OnCompleted; update references to
UploadFileInternal, UploadFileToCommunity, AttachToCommmunity, OnError and
OnCompleted so the outer coroutine yields/returns early (yield break) when the
upload failed.
- Line 530: The log is printing a presigned URL (upload.UploadURI); change the
LogLine.Do call in the upload path (where LogLine.Do($"Uploading {data.Length}
bytes to {upload.UploadURI} (attempt {attempt}/{maxRetries})") is used) to avoid
exposing query/string credentials—either omit upload.UploadURI entirely or
replace it with a redacted form (e.g., Uri.Host or Uri.AbsolutePath and strip
the Query/component) before logging; update the call site in the upload routine
to log only non-sensitive parts (data.Length, attempt/maxRetries, and a redacted
upload identifier) using the LogLine.Do method.
- Around line 692-710: The polling loops currently only check IsFaulted before
accessing task.Result/readTask.Result and thus throw on cancellations; after
each polling loop that waits for a Task to complete (e.g., the
HttpClient.PostAsync task stored in local variable "task" and the
response.Content.ReadAsStringAsync task stored in "readTask", plus the other
similar polling loops in this file), add an IsCanceled check and call OnError
with an appropriate SqEditorApiNetworkException (or propagate the
TaskCanceledException) and yield break before accessing .Result; apply the same
IsCanceled guard pattern to every polling helper loop (the ones that currently
check IsFaulted) to ensure canceled tasks are handled prior to .Result access.
---
Outside diff comments:
In `@Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs`:
- Around line 532-562: Several HTTP request/response objects (HttpRequestMessage
and HttpResponseMessage) created in UploadFileWithRetryAsync,
PostFormEncodedStringNoAuth, JsonGet and JsonPost are not disposed on all
codepaths; update each method to wrap the HttpRequestMessage and the
HttpResponseMessage returned by _httpClient.SendAsync(...) in using blocks (or
ensure disposal in a try/finally) so responses are always disposed before
returning or throwing (including on error/early returns and before any yield
break paths); specifically modify the request creation and the var response =
await _httpClient.SendAsync(...) sites to ensure both request and response are
disposed in every path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a71ed8b-41a6-4a82-959f-63a2f7aaa2dc
📒 Files selected for processing (2)
Editor/Resources/Builder/SideQuest/SqEditorAppApi.csEditor/Resources/Builder/SideQuest/SqEditorModels.cs
| yield return UploadFileInternal(_uploadRequest, data, name, (text) => { }, OnError); | ||
|
|
||
| yield return AttachToCommmunity(() => OnCompleted?.Invoke(_uploadRequest), OnError, _uploadRequest.CommunitiesId, _uploadRequest.FileId, name, assetType, assetPlatform); |
There was a problem hiding this comment.
Stop the outer coroutine when the upload fails.
At Lines 509-513, UploadFileInternal invokes OnError and ends only its own iterator. Lines 475-477 still call AttachToCommmunity, and Lines 492-493 still invoke OnCompleted, so a failed upload can be reported as success and attached anyway.
💡 Propagate the failure back to the callers
public IEnumerator UploadFile(string name, byte[] data, string spaceSlug, Action<SqEditorCreateUpload> OnCompleted, Action<Exception> OnError)
{
SqEditorCreateUpload _uploadRequest = null;
+ var uploadSucceeded = false;
UnityEngine.Debug.Log("Before Upload");
yield return GetUploadRequest((uploadRequest) => _uploadRequest = uploadRequest, OnError, name, data.Length, spaceSlug);
UnityEngine.Debug.Log("After Upload");
if (_uploadRequest == null)
{
OnError?.Invoke(new SqEditorApiException("Failed to get upload request"));
yield break;
}
- yield return UploadFileInternal(_uploadRequest, data, name, (text) => { }, OnError);
+ yield return UploadFileInternal(_uploadRequest, data, name, _ => uploadSucceeded = true, OnError);
+ if (!uploadSucceeded)
+ {
+ yield break;
+ }
OnCompleted?.Invoke(_uploadRequest);
}Apply the same guard in UploadFileToCommunity, or change UploadFileInternal to return a success flag that callers must check.
Also applies to: 492-493, 509-517
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs` around lines 475 - 477,
UploadFileToCommunity currently always proceeds to AttachToCommmunity and
invokes OnCompleted even if UploadFileInternal signaled OnError; change the flow
so failures stop the outer coroutine: have UploadFileInternal communicate
success (e.g., change its contract to return a bool result or set a shared
success flag on the _uploadRequest or a local variable) and in
UploadFileToCommunity check that success before yielding AttachToCommmunity and
before invoking OnCompleted; update references to UploadFileInternal,
UploadFileToCommunity, AttachToCommmunity, OnError and OnCompleted so the outer
coroutine yields/returns early (yield break) when the upload failed.
| try | ||
| { | ||
| LogLine.Do($"Uploading {data.Length} bytes to {url} (attempt {attempt}/{maxRetries})"); | ||
| LogLine.Do($"Uploading {data.Length} bytes to {upload.UploadURI} (attempt {attempt}/{maxRetries})"); |
There was a problem hiding this comment.
Redact the presigned upload URL from logs.
upload.UploadURI is likely a signed URL. Logging it writes temporary credentials into editor logs and bug reports.
🔒 Safer logging
- LogLine.Do($"Uploading {data.Length} bytes to {upload.UploadURI} (attempt {attempt}/{maxRetries})");
+ var safeUploadUri = upload.UploadURI?.Split('?')[0];
+ LogLine.Do($"Uploading {data.Length} bytes to {safeUploadUri} (attempt {attempt}/{maxRetries})");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LogLine.Do($"Uploading {data.Length} bytes to {upload.UploadURI} (attempt {attempt}/{maxRetries})"); | |
| var safeUploadUri = upload.UploadURI?.Split('?')[0]; | |
| LogLine.Do($"Uploading {data.Length} bytes to {safeUploadUri} (attempt {attempt}/{maxRetries})"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs` at line 530, The log is
printing a presigned URL (upload.UploadURI); change the LogLine.Do call in the
upload path (where LogLine.Do($"Uploading {data.Length} bytes to
{upload.UploadURI} (attempt {attempt}/{maxRetries})") is used) to avoid exposing
query/string credentials—either omit upload.UploadURI entirely or replace it
with a redacted form (e.g., Uri.Host or Uri.AbsolutePath and strip the
Query/component) before logging; update the call site in the upload routine to
log only non-sensitive parts (data.Length, attempt/maxRetries, and a redacted
upload identifier) using the LogLine.Do method.
| var content = new StringContent(data, Encoding.UTF8, "application/x-www-form-urlencoded"); | ||
| var task = _httpClient.PostAsync(new Uri(Config.RootApiUri, urlPath), content); | ||
| while (!task.IsCompleted) yield return null; | ||
| if (task.IsFaulted) | ||
| { | ||
| req.uploadHandler = new UploadHandlerRaw(Encoding.UTF8.GetBytes(data)) | ||
| { | ||
| contentType = "application/x-www-form-urlencoded" | ||
| }; | ||
| req.method = "POST"; | ||
| req.downloadHandler = new DownloadHandlerBuffer(); | ||
|
|
||
| yield return req.SendWebRequest(); | ||
|
|
||
| if (req.result == UnityWebRequest.Result.ConnectionError) | ||
| { | ||
| OnError(new SqEditorApiNetworkException($"Unity Network Error: {req.error}")); | ||
| yield break; | ||
| } | ||
| else if (req.result == UnityWebRequest.Result.ProtocolError) | ||
| { | ||
| if (req.responseCode == 401 || req.responseCode == 403) | ||
| { | ||
| OnError(new SqEditorApiAuthException((int)req.responseCode, $"Unity Http Error: {req.error}")); | ||
| yield break; | ||
| } | ||
| else | ||
| { | ||
| OnError(new SqEditorApiAuthException((int)req.responseCode, $"Unity Http Error: {req.error}")); | ||
| yield break; | ||
| } | ||
| } | ||
|
|
||
| var resStr = req.downloadHandler.text; | ||
| if (string.IsNullOrWhiteSpace(resStr)) | ||
| { | ||
| OnCompleted?.Invoke(default(T)); | ||
| yield break; | ||
| } | ||
| else | ||
| { | ||
| try | ||
| { | ||
| OnCompleted?.Invoke(JsonConvert.DeserializeObject<T>(resStr)); | ||
| yield break; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Debug.WriteLine("Failed deserializing response from API", ex); | ||
| OnError?.Invoke(ex); | ||
| yield break; | ||
| } | ||
| } | ||
| OnError(new SqEditorApiNetworkException(task.Exception.InnerException?.Message ?? task.Exception.Message)); | ||
| yield break; | ||
| } | ||
| var response = task.Result; | ||
| if (!response.IsSuccessStatusCode) | ||
| { | ||
| OnError(new SqEditorApiAuthException((int)response.StatusCode, $"Http Error: {response.ReasonPhrase}")); | ||
| yield break; | ||
| } | ||
| var readTask = response.Content.ReadAsStringAsync(); | ||
| while (!readTask.IsCompleted) yield return null; | ||
| if (readTask.IsFaulted) { OnError(readTask.Exception.InnerException ?? readTask.Exception); yield break; } | ||
| var resStr = readTask.Result; | ||
| if (string.IsNullOrWhiteSpace(resStr)) { OnCompleted?.Invoke(default(T)); yield break; } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In .NET, what happens if code reads Task.Resultafter anHttpClientrequest task is canceled or times out, and shouldTask.IsCanceledbe handled separately fromTask.IsFaulted?
💡 Result:
If an HttpClient request task (e.g., from SendAsync or GetAsync) is canceled—either explicitly via CancellationToken or due to HttpClient.Timeout—the task transitions to TaskStatus.Canceled. Accessing Task.Result on a canceled task throws an AggregateException containing a TaskCanceledException in its InnerExceptions. This behavior is documented in the official .NET documentation for Task.Result. Task.IsCanceled will be true for both explicit cancellation and timeout (since HttpClient.Timeout uses cancellation internally), while Task.IsFaulted will be false, as cancellation is not treated as a fault. Task.IsFaulted is true only for unhandled exceptions other than the specific OperationCanceledException that signals cancellation. Therefore, Task.IsCanceled must be handled separately from Task.IsFaulted: check IsCanceled first (or catch TaskCanceledException/AggregateException) to distinguish cancellation from faults. Do not rely solely on IsFaulted, as canceled tasks are not faulted. Best practice: Avoid Task.Result entirely; use await instead, which propagates TaskCanceledException directly without AggregateException.
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/standard/parallel-programming/task-cancellation
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.task-1.result?view=net-9.0
- 3: https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httpclient.send?view=net-9.0
- 4: https://stackoverflow.com/questions/29179848/httpclient-a-task-was-cancelled
- 5: https://oneuptime.com/blog/post/2025-12-23-handle-httpclient-timeout-exceptions/view
🏁 Script executed:
fd -t f "SqEditorAppApi.cs"Repository: SideQuestVR/BanterSDK
Length of output: 175
🏁 Script executed:
wc -l "Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs"Repository: SideQuestVR/BanterSDK
Length of output: 121
🏁 Script executed:
sed -n '692,710p' "Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs" | cat -nRepository: SideQuestVR/BanterSDK
Length of output: 1315
🏁 Script executed:
sed -n '731,751p' "Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs" | cat -nRepository: SideQuestVR/BanterSDK
Length of output: 1471
🏁 Script executed:
sed -n '781,803p' "Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs" | cat -nRepository: SideQuestVR/BanterSDK
Length of output: 1409
Handle canceled tasks before reading .Result.
The polling helpers only check IsFaulted. HttpClient timeouts and cancellations complete as IsCanceled, so accessing .Result on canceled tasks throws an AggregateException with TaskCanceledException inside, bypassing the error handler.
Add an IsCanceled guard after each polling loop before accessing .Result:
Add IsCanceled check in polling helpers
while (!task.IsCompleted) yield return null;
+if (task.IsCanceled)
+{
+ OnError?.Invoke(new SqEditorApiNetworkException("Request timed out or was cancelled."));
+ yield break;
+}
if (task.IsFaulted)
{
OnError(new SqEditorApiNetworkException(task.Exception.InnerException?.Message ?? task.Exception.Message));
yield break;
}Apply the same pattern to all polling loops before .Result access (lines 692–710, 731–751, 781–803).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs` around lines 692 - 710,
The polling loops currently only check IsFaulted before accessing
task.Result/readTask.Result and thus throw on cancellations; after each polling
loop that waits for a Task to complete (e.g., the HttpClient.PostAsync task
stored in local variable "task" and the response.Content.ReadAsStringAsync task
stored in "readTask", plus the other similar polling loops in this file), add an
IsCanceled check and call OnError with an appropriate
SqEditorApiNetworkException (or propagate the TaskCanceledException) and yield
break before accessing .Result; apply the same IsCanceled guard pattern to every
polling helper loop (the ones that currently check IsFaulted) to ensure canceled
tasks are handled prior to .Result access.
1 participant
Summary by CodeRabbit
New Features
Improvements
Refactor